Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perf/rhythm seeking #28

Merged
merged 9 commits into from
Jul 6, 2024
Merged

Perf/rhythm seeking #28

merged 9 commits into from
Jul 6, 2024

Conversation

emuell
Copy link
Owner

@emuell emuell commented Jul 2, 2024

Speed up rhythm/phrase seeking. See #6

  • when seeking phrases & rhythms batch process event iteration where possible

  • try avoiding as much processing as possible while seeking, by using new "omit" event iter functions and cycle processing functions, which only maintain internal event iter state without actually computing events.

This is WIP, and already ~2x faster in many cases, but unfortunately still not fast enough. Ideal would be speedups of ~8x in normal processing vs seeking. When live editing phrase scripts, the seek function is called quite often for quite long periods of time (minutes up to hours). So this is very performance critical.

@unlessgames As mentioned above, I've also added a new "omit" function to Cycle, but the implementation is rather ugly, but maybe still better than duplicating the whole "output" function for seeking. In general, anything that can be skipped should be skipped there - without breaking the internal state across cycle passes. To make this easier to check, I've added some tests for this, and also reorganised the tests to separate the seek tests from the others.
See especially this change here: 774fde6

You may have some better ideas on how to fix or improve this.

PS: Omit is quite a shitty name for this, but unfortunately "skip" clashes with the standard Rust Iterator impls. I'll clean this skip vs seek vs omit wording after this PR is merged. This will change things across the whole repository. This PR already now is too big.

I'll now concentrate on optimising the other parts of the rhythms: generator patterns, emitters.

@emuell emuell force-pushed the perf/rhythm-seeking branch from 869558e to 774fde6 Compare July 2, 2024 18:54
Repository owner deleted a comment from github-actions bot Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

Benchmark for 03cbd37

Click to view benchmark
Test Base PR %
Cycle/Generate 59.1±0.64µs 58.9±0.76µs -0.34%
Cycle/Parse 328.3±3.46µs 337.3±6.76µs +2.74%
Rust Phrase/Clone 1092.8±62.69ns 1070.1±20.20ns -2.08%
Rust Phrase/Create 68.2±1.10µs 68.9±0.83µs +1.03%
Rust Phrase/Run 709.6±8.06µs 706.5±4.26µs -0.44%
Rust Phrase/Seek 12.6±0.05ms 9.4±0.08ms -25.40%
Scripted Phrase/Clone 1174.3±13.36ns 1147.3±14.55ns -2.30%
Scripted Phrase/Create 783.1±13.01µs 777.8±9.27µs -0.68%
Scripted Phrase/Run 1731.7±9.63µs 1762.0±39.44µs +1.75%
Scripted Phrase/Seek 35.6±0.21ms 15.2±0.80ms -57.30%

@emuell
Copy link
Owner Author

emuell commented Jul 3, 2024

See especially this change here: 774fde6

I'm stupid and forgot the discussion we already had about this :)

The only state that needs to be managed here is the random state of the cycle, which actually only is needed when the cycle is seeded. All other state is derived from the cycle's iteration count. So this commit can be removed.

Instead, omit should always increase the cycle iteration count, and seeded cycles should derive the random number generator state from the cycle iteration count once on each new generate call.

@emuell emuell force-pushed the perf/rhythm-seeking branch from 774fde6 to 322f760 Compare July 3, 2024 06:54
Copy link

github-actions bot commented Jul 3, 2024

Benchmark for 6227750

Click to view benchmark
Test Base PR %
Cycle/Generate 58.7±0.68µs 59.2±1.25µs +0.85%
Cycle/Parse 326.7±4.40µs 342.0±29.76µs +4.68%
Rust Phrase/Clone 1084.4±8.29ns 1009.6±17.77ns -6.90%
Rust Phrase/Create 68.1±1.41µs 70.2±0.70µs +3.08%
Rust Phrase/Run 705.1±15.69µs 704.9±7.48µs -0.03%
Rust Phrase/Seek 12.2±0.04ms 9.2±0.03ms -24.59%
Scripted Phrase/Clone 1166.3±9.45ns 1041.7±16.15ns -10.68%
Scripted Phrase/Create 789.6±6.97µs 794.2±11.76µs +0.58%
Scripted Phrase/Run 1769.6±37.84µs 1762.5±41.83µs -0.40%
Scripted Phrase/Seek 36.0±0.25ms 13.4±0.07ms -62.78%

@emuell emuell force-pushed the perf/rhythm-seeking branch from 322f760 to 0e3e93b Compare July 3, 2024 19:26
@emuell emuell marked this pull request as ready for review July 3, 2024 19:26
@emuell emuell requested a review from unlessgames July 3, 2024 19:26
Copy link

github-actions bot commented Jul 3, 2024

Benchmark for 1d73755

Click to view benchmark
Test Base PR %
Cycle/Generate 59.3±0.80µs 59.2±0.90µs -0.17%
Cycle/Parse 320.2±5.34µs 323.2±5.94µs +0.94%
Rust Phrase/Clone 1077.3±13.80ns 1018.4±7.81ns -5.47%
Rust Phrase/Create 66.3±1.02µs 67.9±2.85µs +2.41%
Rust Phrase/Run 704.7±7.75µs 694.4±3.14µs -1.46%
Rust Phrase/Seek 300.5±565.68µs 160.4±301.95µs -46.62%
Scripted Phrase/Clone 1160.8±17.64ns 967.9±13.04ns -16.62%
Scripted Phrase/Create 851.4±14.65µs 870.0±16.39µs +2.18%
Scripted Phrase/Run 1726.3±10.63µs 1868.4±29.82µs +8.23%
Scripted Phrase/Seek 6.6±7.90ms 250.2±509.87µs -96.21%

@emuell emuell mentioned this pull request Jul 3, 2024
4 tasks
@unlessgames
Copy link
Collaborator

The random output in the cycle currently has a regression in that it is no longer being deterministic based on the seed, since I've added the proper pattern mult and division.

Seeding once per iteration will still not be deterministic enough if we want to mirror tidalcycles. Here is a quote from my previous comment on this

[[a b] | [c d]]/2

Here tidal will output a single event per cycle, but b will always follow a and d will always follow c since cycles are queried in whole and their randomness is a pure function of the iteration count of the (sub)cycle, the step and the seed.

I've added the step variable to the state (that you are removing here) to prepare for an eventual solution because it is no longer sufficient to have a starting seed as the multiply operator has to sample the subcycle it affects at arbitrary iterations to get the output.

So apart from a top level seed we would need the random generator to be sampled by position instead of just yielding some value when gen_range is called. Ideally we could be able to say rng.sample(iteration + step) where step is the absolute position index of the currently outputting step in the entire pattern or something similar. I might be missing something here but I think this would solve it.

@emuell
Copy link
Owner Author

emuell commented Jul 5, 2024

The current behaviour is indeed still not correct (not behaving like the tidal reference implementation), but this is somewhat out of scope here. I would make this a follow-on TODO/PR, okay?

Ideally we could be able to say rng.sample(iteration + step) where step is the absolute position index of the currently outputting step in the entire pattern or something similar.

With the introduction of step, the new advance function will actually have to use a function similar to generate to maintain the step, which will then depend on the random state. That's what confused me at first.

And If we're going to change that, it might be easier and more efficient to use a simpler PRNG, where the state index can be set directly, rather than having to reseed it every time. This should be possible with e.g. a simple Mersenne Twister.

@emuell
Copy link
Owner Author

emuell commented Jul 5, 2024

@unlessgames okay for you to close and merge this? Then I can move on from here...

@unlessgames
Copy link
Collaborator

Sounds good! Maybe it will be best to implement this once the last feature for tidal is complete.

@emuell emuell merged commit b891254 into master Jul 6, 2024
2 checks passed
@emuell emuell deleted the perf/rhythm-seeking branch July 6, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants